Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: foo20 check an error from grc20 package #977

Merged
merged 2 commits into from
Jul 20, 2023

Conversation

r3v4s
Copy link
Contributor

@r3v4s r3v4s commented Jul 17, 2023

detailed infos are described in #857

TL;DR
foo20 relam doesn't check error from grc20 package.

  • for example sending to address itself should fail, but currently runs fine
Checklists...

Contributors Checklist

  • Added new tests, or not needed, or not feasible
  • Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
  • Updated the official documentation or not needed
  • No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
  • Added references to related issues and PRs
  • Provided any useful hints for running manual tests
  • Added new benchmarks to generated graphs, if any. More info here.

Maintainers Checklist

  • Checked that the author followed the guidelines in CONTRIBUTING.md
  • Checked the conventional-commit (especially PR title and verb, presence of BREAKING CHANGE: in the body)
  • Ensured that this PR is not a significant change or confirmed that the review/consideration process was appropriate for the change

@r3v4s r3v4s requested a review from a team as a code owner July 17, 2023 10:19
@github-actions github-actions bot added the 🧾 package/realm Tag used for new Realms or Packages. label Jul 17, 2023
@moul
Copy link
Member

moul commented Jul 18, 2023

Related with #952

examples/gno.land/r/demo/foo20/foo20_test.gno Outdated Show resolved Hide resolved

func TestErrConditions(t *testing.T) {
admin := std.Address("g1us8428u2a5satrlxzagqqa5m6vmuze025anjlj")
empty := std.Address("")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add another case?

	invalid := std.Address("invalid")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

grc20 package uses checkIsValidAddress to check whether address is valid or not.

func checkIsValidAddress(addr std.Address) error {
if addr.String() == "" {
return ErrInvalidAddress
}
return nil
}

unfortunately it only checks addr.String() == "", so that invalid won't be invalid but valid.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps better to rename above function to checkIsEmptyAddress

Copy link
Contributor

@waymobetta waymobetta Jul 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ethereum ecosystem libs like Ethers and web3.js commonly utilize the isAddress terminology, if the aim is for close parity or to minimize confusion errors for devs.

SO answer with additional context.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's handle this in the other refactor #952.

In this particular case, using isAddress doesn't seem suitable because we're dealing with an std.Address object and checking its validity.

Creating a more general method like std.Address{}.IsValid() would be more appropriate in the future. For now, I'll focus on improving the check in the other PR or add a comment for clarification.

Thank you all for your suggestions.

@moul moul merged commit 346359e into gnolang:master Jul 20, 2023
@moul moul deleted the feat/foo20-error-handler branch July 20, 2023 11:21
Doozers pushed a commit to Doozers/gno that referenced this pull request Aug 31, 2023
@moul moul added this to the 🌟 main.gno.land (wanted) milestone Sep 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🧾 package/realm Tag used for new Realms or Packages.
Projects
Archived in project
Status: Done
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants